Worker Deployment Versioning #1679
Conversation
There was a problem hiding this comment.
Didn't really look through the core bridge stuff, maybe worth coordinating on #1638.
For your workflow test files deployment-versioning-no-annotations... there's a workflows directory in test/src that might make more sense for them to be located, instead of directly in test/src.
Just want to re-affirm how this works:
- on the worker, we have the worker's configured deployment version, and the default versioning behaviour for all its workflows
- on the workflow, we can override this versioning behaviour with the newly added workflow definition options (it's a little funky, we're making use of JS's quirk that a function is an object, so a workflow with definition options is a function with properties, as already discussed)
- workflow versioning behaviour is read into the activator (which uses it solely when concluding the activation) and workflow info
Tests were helpful, though I think you probably want someone whose more familiar with the versioning behaviour to take a look
|
|
||
| const test = makeTestFunction({ workflowsPath: __filename }); | ||
|
|
||
| test('Worker deployment based versioning', async (t) => { |
There was a problem hiding this comment.
IIUC, there isn't any actual upgrading going on in this test? This test is checking that workflows with different versioning behaviours can run on workers with different deployment options?
There was a problem hiding this comment.
It definitely does. The wf1 workflow starts on v1 and ends on v3
Sushisource
force-pushed
the
worker-deployment-versioning
branch
4 times, most recently
from
a120422 to
e59e2d0
Compare
Sushisource
force-pushed
the
worker-deployment-versioning
branch
from
a8d4a69 to
f18b1ca
Compare
| @@ -79,7 +79,21 @@ export function initRuntime(options: WorkflowCreateOptionsInternal): void { | |||
| const workflowFn = mod[activator.info.workflowType]; | |||
| const defaultWorkflowFn = mod['default']; | |||
|
|
|||
| if (typeof workflowFn === 'function') { | |||
| if (isWorkflowFunctionWithOptions(workflowFn)) { | |||
There was a problem hiding this comment.
We're loosing some important type checking here, and making the "default export with options" win over "exact named function without options", which seems undesirable.
Keep the (typeof workflowFn === 'function') and (typeof defaultWorkflowFn === 'function') at the top level, and move the check for options inside those top level branches.
Also, in the else branch of workflowFn.options, make sure that it is indeed a function before assigning it to workflowDefinitionOptionsGetter.
|
This will need to hang out for a bit since I want temporalio/api#579 in before I incorporate that |
Sushisource
force-pushed
the
worker-deployment-versioning
branch
2 times, most recently
from
218743e to
8169797
Compare
Sushisource
force-pushed
the
worker-deployment-versioning
branch
4 times, most recently
from
e79e306 to
3ee553e
Compare
| @@ -253,3 +256,9 @@ export function extractWorkflowType<T extends Workflow>(workflowTypeOrFunc: stri | |||
| `Invalid workflow type: expected either a string or a function, got '${typeof workflowTypeOrFunc}'` | |||
| ); | |||
| } | |||
|
|
|||
| /* eslint-disable @typescript-eslint/explicit-module-boundary-types */ | |||
| export function isWorkflowFunctionWithOptions(obj: any): obj is WorkflowFunctionWithOptions<any[], any> { | |||
There was a problem hiding this comment.
Is there some rationale for adding this function in this file/package, even though it is only used from workflow/worker-interface.ts?
| /** | ||
| * A workflow function that has been defined with options from {@link WorkflowDefinitionOptions}. | ||
| */ | ||
| export interface WorkflowFunctionWithOptions<Args extends any[], ReturnType> extends AsyncFunction<Args, ReturnType> { |
There was a problem hiding this comment.
Does this have any value being exported publicly? If yes, then AsyncFunction should also be exported. If not, then please add @internal and @hidden (I will have to experiment a bit more to figure out how to reduce this to a single annotation, but in the mean time, let's use both).
| const test = makeTestFunction({ workflowsPath: __filename }); | ||
|
|
||
| test('Worker deployment based versioning', async (t) => { | ||
| const taskQueue = 'worker-deployment-based-versioning-' + randomUUID(); |
There was a problem hiding this comment.
nit: Next time, consider using the const { startWorkflow, createWorker, client, taskQueue } = helpers(t) function. That can save quite a few lines of boilerplate.
There was a problem hiding this comment.
I couldn't use those helpers for reasons I can't remember perfectly, now - I think it had to do with the fact that I need multiple workers and need them all to use the same TQ but different other options.
| export type ExistingServerTestWorkflowEnvironmentOptions = { | ||
| /** If not set, defaults to localhost:7233 */ | ||
| address?: string; | ||
| client?: ClientOptions; |
There was a problem hiding this comment.
Why ClientOptions rather than ClientOptionsForTestEnv like other variants? I'm particularly worried about the fact that this would make it possible for caller to pass in an existing Connection, which would override (line 96) the one we create at line 222.
There was a problem hiding this comment.
Really just because you need namespace - but I can add that in to the type explicitly
Sushisource
force-pushed
the
worker-deployment-versioning
branch
from
a8b2c1f to
0382bdf
Compare
Sushisource
force-pushed
the
worker-deployment-versioning
branch
from
0382bdf to
48bbb74
Compare
Sushisource
force-pushed
the
worker-deployment-versioning
branch
from
48bbb74 to
77cd10e
Compare
What was changed
Added the annotations and options for worker-deployment-based versioning
Why?
All aboard the versioning train
Checklist
Closes [Feature Request] Support New Worker Versioning API #1659
How was this tested:
Added tests
Any docs updates needed?